Fixed multiple configNode bugs - #17609

WAL replay idempotency requirement

With the new write-ahead ordering (persist → execute), if persistPlanForSimpleConsensus succeeds at line 125 but executor.executeNonQueryPlan fails at line 133, the plan is already persisted in WAL and will be replayed on restart.

This is correct WAL semantics, but it introduces a hard requirement: all ConfigPhysicalPlan implementations must be idempotent under replay. For example, a RegisterDataNodePlan replayed against an already-registered node must not fail or double-register.

Is this idempotency property currently guaranteed across all ConfigPhysicalPlan types? If not, this could cause issues on crash recovery in edge cases.

Addressed in cddb4b1. A plan whose execution fails is now removed from the SimpleConsensus WAL by truncating and force-syncing back to the pre-write offset, so normal rollback leaves nothing to replay. ConfigRegionStateMachineTest.testFailedSimpleConsensusWriteRollsBackPersistedPlan verifies the WAL contains no replayable plan. For the registration-related plans touched here, NodeInfoTest.testRegistrationPlansAreIdempotentForWalReplay also executes RegisterDataNodePlan, ApplyConfigNodePlan, and UpdateVersionInfoPlan twice and verifies stable state. If truncation itself fails, the status and operator log explicitly warn that replay remains possible.

force() on every write makes scheduled flush redundant

Now that every write is synchronously forced here, the scheduled flushWALForSimpleConsensus thread (lines 447-455) becomes redundant — it calls simpleLogWriter.force() periodically, but there's nothing left to flush.

This is harmless (double-force is idempotent) but worth either:

1. Removing the scheduled thread to avoid confusion, or
2. Adding a brief comment explaining the intentional belt-and-suspenders approach.

For ConfigNode metadata operations (low write frequency), the per-write force() is the right durability choice.

Addressed. The scheduled flush executor and flushWALForSimpleConsensus method were removed; each persisted write is synchronously forced, and the writer is now closed from stop().

Nit: String.valueOf(e.getMessage()) is redundant — string concatenation with + already handles null by converting to "null". Can simplify to:

"Persist ConfigNode SimpleConsensus log failed: " + e.getMessage()

Addressed in the rewritten persistence error path: the message now directly concatenates e.getMessage().

Good fix — synchronized copy before iterating. One minor observation: this copies the entire list under the lock. If regionMaintainTaskList can grow very large, this could cause a brief pause. In practice, for ConfigNode this list is typically small, so this is fine. Just noting for future awareness.

Thanks for noting this. The snapshot path intentionally copies under the lock to obtain a consistent list, then performs serialization after releasing the lock. As noted, this list is expected to remain small for ConfigNode.

Is this a bug?

Yes, this was an off-by-one side effect: getAndIncrement() returned the correct count but unnecessarily advanced the local counter before returning. It now uses get(), and PartitionInfoTest.testRegionGroupCount covers both schema and data region counts.

Correction to my earlier reply: result is a local counter, and getAndIncrement() returns its value before incrementing it. Replacing it with get() leaves the returned count unchanged; this removes an unused increment rather than fixing an off-by-one bug. I have corrected item 4 in the PR description accordingly.

With persist before execute, a successful persistPlanForSimpleConsensus followed by a failed executeNonQueryPlan leaves the plan in WAL and it will be replayed on restart. Every ConfigPhysicalPlan executed on replay must be idempotent. Please add an audit or tests for the plan types touched by this PR.

Addressed. The failed-write test audits the persist-success/execute-failure path and verifies rollback leaves no replayable WAL entry. I also added replay idempotency coverage for the plan types directly touched by the registration changes: RegisterDataNodePlan, ApplyConfigNodePlan, and UpdateVersionInfoPlan.

rollbackFailedPlanForSimpleConsensus on execute failure is the right safety net. Ensure rollback failure paths are logged clearly enough for operators when WAL and in-memory state may diverge.

Addressed. Rollback failure logging now includes the plan type, WAL file, truncate offset, and pre-write end index. The returned status also explicitly says that the persisted plan may replay after restart, so both operators and callers can see the possible WAL/in-memory divergence.

[P1] Remove the appended WAL record when persistence fails

• If simpleLogWriter.write(buffer) succeeds but force() throws, this catch returns a failure without truncating the record already appended to the WAL.
• write() then returns before calling the executor, but restart recovery replays the record, so an operation rejected to the caller takes effect after restart. A partial append can also leave an invalid record ahead of subsequent writes.
• I reproduced the complete-record case with an injected force() failure: status 301, zero executor calls, and a 36-byte WAL record that SingleFileLogReader successfully replays as UpdateVersionInfo.

Please restore and force-sync the saved pre-write offset on persistence failure, and prevent further writes if that cleanup cannot complete. Add coverage for append/force failures as well as executor failures.

Fixed in 1e1ffe04c8d. Append and force() failures now restore the saved pre-write WAL offset and force-sync the truncation before another write is accepted. If cleanup cannot complete, further SimpleConsensus writes are rejected until restart, with an explicit status and operator log.

Regression tests cover a complete appended record followed by a force failure, a partial append failure, and cleanup failure, in addition to the existing executor-failure test. The replay checks preserve the preceding accepted plan, exclude the failed plan, and verify that a subsequent write succeeds after successful cleanup.

[P1] Avoid decommissioning the joining ConfigNode during registration rollback

• At this point the joining ConfigNode is already a Raft member: AddConfigNodeProcedure completes ADD_PEER before entering REGISTER_SUCCESS. It therefore also applies this RemoveConfigNodePlan.
• NodeInfo.removeConfigNode() sets the local node status to Removing when the removed ID is its own. The procedure retries REGISTER_SUCCESS after this exception, but neither applyConfigNode() nor notifyRegisterSuccess() resets that status.
• A transient version-write failure followed by a successful retry consequently leaves the newly registered ConfigNode rejecting requests in ConfigManager.confirmLeader().

Please use a registration rollback that avoids node-removal lifecycle side effects, or explicitly restore the appropriate node status when registration succeeds. A regression test should cover version-write failure followed by a successful registration retry.

Fixed in 1e1ffe04c8d. A failed ConfigNode version write no longer submits RemoveConfigNodePlan. Since ADD_PEER has already joined the node to Raft, its membership is retained and the failure is propagated so REGISTER_SUCCESS can retry the idempotent registration and version plans. Heartbeat-cache creation remains after both writes succeed.

The regression test applies the plans through a real NodeInfo as the joining node. It verifies that the node remains Running after the version-write failure and after a successful retry, registration contains one member, the version is updated, and no removal plan is written.

[P2] Localize the new registration failure templates

• This template, the rollback result messages/logs below, and the analogous AINode/ConfigNode helpers introduce raw English strings in Java code. The new exception/status descriptions and the constants near the top of this class have the same issue.
• These strings remain English under -P with-zh-locale, because that build only swaps the locale-specific message classes.

Please put each complete user-visible or operator-visible template in both the English and Chinese ConfigNodeMessages classes, preserve its format placeholders, and reference those constants here. This also applies to the new rollback log and exception templates.

Fixed in 1e1ffe04c8d. The registration failure statuses, rollback logs, exception templates, and fallback status descriptions now use matching English and Chinese ConfigNodeMessages constants. Each outcome uses a complete format template, and the raw English fragment constants were removed. The new WAL failure messages are also localized.

Full-reactor test compilation passed for both locales; all 431 ConfigNode message keys and placeholder counts match. A scan found no added raw English literals in main Java, and all 12 focused WAL/registration tests passed in both locales.